[CALCITE-7646] `CorrelateProjectExtractor` does not handle nested field accesses `cor0.field0.field1` by mihaibudiu · Pull Request #5086 · apache/calcite

mihaibudiu · 2026-07-06T21:48:34Z

Jira Link

CALCITE-7646

Changes Proposed

These changes make the helper class slightly more capable, enabling more programs to be decorrelated.

mihaibudiu · 2026-07-06T21:51:35Z

@zabetak according to git blame you wrote most of this code, so I took the liberty to request your review.

mihaibudiu · 2026-07-06T23:37:38Z

I think the CI failure is transient, the tests pass locally.

xuzifu666 · 2026-07-07T02:53:18Z

I think the CI failure is transient, the tests pass locally.

There shouldn't be any problems. I re-trigger sonar CI to double-check.

mihaibudiu · 2026-07-11T01:16:09Z

The fact that I requested a review from @zabetak should not deter anyone else from reviewing this PR. I think it's a genuine improvement which will help our decorrelator handle more queries.

iwanttobepowerful · 2026-07-11T04:48:28Z

Would it also be possible to add a few SQL-based test cases to QuidemTest?

mihaibudiu · 2026-07-11T05:01:43Z

Sure, I will do that, but the results should not change, just the nature of the plan

xuzifu666

In my view the overall implementation is ok and meets the objectives. Apart from suggesting the addition of QuidemTest, I have left two comments for your reference.

mihaibudiu · 2026-07-14T00:17:01Z

Would it also be possible to add a few SQL-based test cases to QuidemTest?

I have added a few quidem tests covering both decorrelators.

mihaibudiu · 2026-07-14T00:18:46Z

Interestingly, Claude actually found a (related) bug in the original optimization, which forgot to preserve fields NOT moved into requiredColumns. Several test cases cover these failures. I haven't filed a separate issue since there is common code between this "improvement" and the bug fix itself. As a consequence of this fix several plans have changed, but they are equivalent and always better.

zabetak

Overall LGTM! Left some small comments/suggestions but nothing major.

zabetak · 2026-07-14T14:20:25Z

+    if (node instanceof RexFieldAccess) {
+      RexFieldAccess access = (RexFieldAccess) node;
+      if (access.getReferenceExpr() instanceof RexCorrelVariable) {
+        RexCorrelVariable var = (RexCorrelVariable) access.getReferenceExpr();


var is a reserved keyword in JDK 10+ so to avoid any potential confusion I think its worth addressing this minor sonar warning.

zabetak · 2026-07-14T14:39:11Z

    List<RexNode> callsWithCorrelationOverLeft = new ArrayList<>();
    for (RexNode callInRight : callsWithCorrelationInRight) {
-      callsWithCorrelationOverLeft.add(replaceCorrelationsWithInputRef(callInRight, builder));
+      if (isDirectFieldAccess(callInRight, correlate.getCorrelationId())) {


How important is to have this condition? I understand that it tries to block "trivial" correlated expressions from generating redundant column references but it shouldn't lead to incorrect plans.

Actually this is the bug that leads to the two crashes highlighted in some of the tests.
You need to put the field in the list of required columns.

zabetak · 2026-07-14T14:52:09Z

+# 4 test cases for [CALCITE-7646] CorrelateProjectExtractor
+# does not handle nested field accesses cor0.field0.field1.
+
+# All queries use LATERAL, which converts directly to a Correlate.


Since the queries make use of lateral they fit better in the lateral.iq file.

zabetak · 2026-07-14T14:56:52Z

+!use scott
+
+# Before the fix planning failes with
+# "Mappings$NoElementException: source #0 has no target in mapping".


The problem/failure (along with stacktrace information) are much more useful in the Jira description than in comments in the code. Users encountering a problem will first search on Google/Jira before digging into the code.

For reviewers its also easier to understand the context of the work by reading the problem description first.

I will move the error descriptions in Jira and delete them from the comments

zabetak · 2026-07-14T15:03:10Z

+!if (use_new_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], expr#4=['SALES':VARCHAR(14)], expr#5=[=($t1, $t4)], DD=[$t2], X=[$t3], $condition=[$t5])
+  EnumerableCorrelate(correlation=[$cor1], joinType=[inner], requiredColumns=[{0}])
+    EnumerableCalc(expr#0..2=[{inputs}], proj#0..1=[{exprs}])
+      EnumerableTableScan(table=[[scott, DEPT]])
+    EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO], DD=[$t2], X=[$t0])
+      EnumerableUncollect
+        EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO], expr#3=[100], expr#4=[+($t2, $t3)], expr#5=[ARRAY($t4)], EXPR$0=[$t5])
+          EnumerableValues(tuples=[[{ 0 }]])
+!plan
+!}


The new decorrelator is not using the CorrelateProjectExtractor so to some extend this test is unrelated to the fix and somewhat redundant. Is there something specific that we want to check in the plan?

zabetak · 2026-07-14T15:22:51Z

+!if (use_old_decorr) {
+EnumerableCalc(expr#0..3=[{inputs}], DD=[$t2], X=[$t3])
+  EnumerableCorrelate(correlation=[$cor1], joinType=[inner], requiredColumns=[{0, 1}])
+    EnumerableCalc(expr#0..2=[{inputs}], expr#3=[100], expr#4=[+($t0, $t3)], expr#5=[ARRAY($t4)], expr#6=['SALES':VARCHAR(14)], expr#7=[=($t1, $t6)], DEPTNO=[$t0], $f3=[$t5], $condition=[$t7])
+      EnumerableTableScan(table=[[scott, DEPT]])
+    EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.DEPTNO], DD=[$t2], X=[$t0])
+      EnumerableUncollect
+        EnumerableCalc(expr#0=[{inputs}], expr#1=[$cor1], expr#2=[$t1.$f3], EXPR$0=[$t2])
+          EnumerableValues(tuples=[[{ 0 }]])
+!plan


Apart from the fact that the query runs (does not crash) and produces correct results what do we want to assert/verify in the query plan?
If we don't really care about the query plan then we should skip it. If there is something that we need to validate then an extra comment would be helpful.

zabetak · 2026-07-14T15:26:47Z

+
+# EnumerableRules.ENUMERABLE_CORRELATE_RULE is removed so that this test
+# fails if the plan cannot be fully decorrelated.
+!set planner-rules "-EnumerableRules.ENUMERABLE_CORRELATE_RULE"


Nice idea for ensuring that a plan is fully decorrelated although such test would fit better in RelDecorrelatorTest where it would be more readable.

… accesses cor0.field0.field1 Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu · 2026-07-14T23:02:13Z

@zabetak I have temporarily pushed the changes requested by your review as a second commit, for you to check.
I plan to squash and merge eventually.

sonarqubecloud · 2026-07-14T23:35:36Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu force-pushed the issue7646 branch from 9539521 to 82d3362 Compare July 6, 2026 21:50

mihaibudiu requested a review from zabetak July 6, 2026 21:51

mihaibudiu force-pushed the issue7646 branch 2 times, most recently from 9f0f80c to d5bd389 Compare July 6, 2026 23:08

mihaibudiu mentioned this pull request Jul 6, 2026

[SQL] Support for new Calcite UNNEST operator feldera/feldera#6599

Draft

1 task

xuzifu666 reviewed Jul 12, 2026

View reviewed changes

Comment thread core/src/main/java/org/apache/calcite/sql2rel/CorrelateProjectExtractor.java Outdated

Comment thread core/src/test/java/org/apache/calcite/sql2rel/CorrelateProjectExtractorTest.java

mihaibudiu force-pushed the issue7646 branch from d5bd389 to 2e1866b Compare July 14, 2026 00:16

xuzifu666 approved these changes Jul 14, 2026

View reviewed changes

zabetak approved these changes Jul 14, 2026

View reviewed changes

mihaibudiu added 3 commits July 14, 2026 14:49

[CALCITE-7646] CorrelateProjectExtractor does not handle nested field…

74ad753

… accesses cor0.field0.field1 Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Address review comments

066a6e7

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

Address review comments

91ccf26

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>

mihaibudiu force-pushed the issue7646 branch from 2e1866b to 91ccf26 Compare July 14, 2026 23:01

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Jul 14, 2026

Uh oh!

Conversation

mihaibudiu commented Jul 6, 2026

Jira Link

Changes Proposed

Uh oh!

mihaibudiu commented Jul 6, 2026

Uh oh!

mihaibudiu commented Jul 6, 2026

Uh oh!

xuzifu666 commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mihaibudiu commented Jul 11, 2026

Uh oh!

iwanttobepowerful commented Jul 11, 2026

Uh oh!

mihaibudiu commented Jul 11, 2026

Uh oh!

xuzifu666 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mihaibudiu commented Jul 14, 2026

Uh oh!

mihaibudiu commented Jul 14, 2026

Uh oh!

zabetak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Jul 14, 2026

Uh oh!

sonarqubecloud Bot commented Jul 14, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xuzifu666 commented Jul 7, 2026 •

edited

Loading